Add PUT /users/:id (update a user) - #130
Open
tvk777 wants to merge 4 commits into
Open
Conversation
Adds an updateUser(id, fields) function to the in-memory store, following the existing getUserById/createUser pattern. It reuses getUserById for the lookup, replaces name and email in place, and returns undefined for an unknown id so route handlers stay in charge of the HTTP response. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a full-replacement update endpoint to the users resource. It validates that name and email are present, non-empty strings before touching the store (400 otherwise), rejects a non-numeric id with a 400, and returns 404 when no user has that id instead of crashing. Data access goes through the store's new updateUser helper, matching the other handlers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Records the decisions behind the update-user change: the plan and the two questions settled before approving it, the model choice, why the commits were split store-then-route, and what the self-review caught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Number() coerces "0x2", "+2", "2e0", "2.0" and a URL-encoded " 2" all to 2, so those forms bypassed the id guard and updated user 2 with a 200 despite the handler claiming to require a number. Match the raw param against /^\d+$/ before converting, and note the finding in NOTES.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds the missing "update a user" endpoint to the users resource:
PUT /users/:id.db/store.js— newupdateUser(id, { name, email })helper alongside the existinggetUserById/createUser. It reusesgetUserByIdfor the lookup, replaces the fieldsin place, and returns
undefinedfor an unknown id so the store stays free of HTTP concerns.routes/users.js— thePUT /users/:idhandler. Full-replacement semantics: bothnameandemailare required. All data access goes through the store, matching theother handlers.
NOTES.md— the decisions behind the change (plan, model choice, commit split, review).Why
The users resource supported list, get-by-id and create, but there was no way to update a
user.
tests/update-user.test.jsalready specified the contract and was red; this makes itgreen without touching the test file.
Behavior
200+ the updated user404 {"error":"User not found"}nameoremail400 {"error":"name and email are required"}400 {"error":"id must be a number"}Two deliberate choices worth a reviewer's attention:
400and never a partially applied write. The tests don't exercise that conflicting case,so the ordering is a decision rather than something the suite pins down.
POST /usersdoesn't validate format either, and having createand update disagree about what a valid email is would be worse than having neither check.
What a reviewer should test
Then against a running server (
npm run dev):curl -X PUT localhost:3000/users/1 -H 'content-type: application/json' -d '{"name":"Ada L.","email":"ada@new.com"}'→ 200;GET /users/1andGET /usersboth show the new values.PUT /users/9999with a valid body → 404, not a crash.PUT /users/1 -d '{"name":"only a name"}'→ 400.{"name": 42}, and an absent body → 400 rather than a 500.PUT /users/abc→ 400.PUT /users/0x2→ 400 (see below).Note on the id guard
A self-review caught that
Number(req.params.id)is laxer than its own error message:0x2,+2,2e0,2.0and a URL-encoded%202all coerce to2, soPUT /users/0x2returned a 200 and overwrote user 2. The provided tests wouldn't catch it — they only try a
well-formed id and a fully non-numeric one. The last commit matches the raw param against
/^\d+$/before converting.Out of scope
POST /usersstill uses a bare truthiness check, so it accepts{"name": 42}andwhitespace-only names — create and update now disagree slightly on field validity. That's
pre-existing code this change doesn't touch; happy to tighten it in a follow-up.
🤖 Generated with Claude Code